fix(dns): Fix response buffer race condition in concurrent queries by abhik-roy85 · Pull Request #1054 · espressif/esp-protocols

abhik-roy85 · 2026-05-04T12:17:56Z

Description

Fixes a race condition when multiple getaddrinfo() calls run concurrently through the esp_dns lwIP hook (DoT/TCP/DoH). Also addresses review feedback on configurability, examples, and test layout.

Component changes

Concurrent lookups: Move response_buffer_t to stack-local storage in dns_resolve_doh, dns_resolve_dot, and dns_resolve_tcp. DoH passes the per-query buffer via esp_http_client_config_t::user_data so overlapping HTTP events do not share state.
CNAME handling: Decouple answer RR scan limit from returned IP count. Walk up to CONFIG_ESP_DNS_MAX_ANSWER_SCAN RRs while collecting addresses, so CNAME + A/AAAA in one response works when MAX_ANSWERS is 1.
Parsing cleanup: Store ip_addr_t directly in dns_response_t; rename esp_dns_extract_ip_addresses_from_response → esp_dns_get_ips_from_response.
Caller buffer safety: dns_resolve_* take addr_cnt; esp_dns_parse_response() and esp_dns_get_ips_from_response() take max_ips / max_entries, clamped via esp_dns_clamp_addr_cnt(). The lwIP hook passes MAX_ANSWERS (1) until LWIP_HOOK_NETCONN_EXTERNAL_RESOLVE forwards the caller's addr_cnt.
Kconfig: Add CONFIG_ESP_DNS_MAX_ANSWER_SCAN and CONFIG_ESP_DNS_BUFFER_SIZE under Component config → ESP DNS.
The public API (include/esp_dns.h) is unchanged. Parse/extract helpers remain component-private.

Examples and tests

Migrate esp_dns_basic and the concurrent stress app from protocol_examples_common to net_connect (separate commit from component parsing changes).
Enable CONFIG_MBEDTLS_CERTIFICATE_BUNDLE_CROSS_SIGNED_VERIFY in example/test sdkconfig.defaults; document opt-out in READMEs.
Increase esp_dns_basic getaddrinfo worker stack to 8192 bytes by default; make it configurable via CONFIG_EXAMPLE_DNS_ADDRINFO_TASK_STACK.
Move concurrent stress app from examples/esp_dns_concurrent_test to tests/test_apps/esp_dns_concurrent.
Extend esp_dns__build.yml CI to build both esp_dns_basic and esp_dns_concurrent.

Testing

Checklist

Before submitting a Pull Request, please ensure the following:

🚨 This PR does not introduce breaking changes.
All CI checks (GH Actions) pass.
Documentation is updated as needed.
Tests are updated or added as necessary.
Code is well-commented, especially in complex areas.
Git history is clean — commits are squashed to the minimum necessary.

Note

Medium Risk
Touches core DNS resolution paths used by getaddrinfo() and TLS/HTTP clients; behavior changes are intentional but concurrent networking code warrants careful review.

Overview
Fixes a race when multiple getaddrinfo() calls run through the esp_dns lwIP hook for TCP/DoT/DoH by removing shared response_buffer_t from the DNS handle and keeping per-lookup state on the stack. DoH wires that buffer through esp_http_client user_data so overlapping HTTP callbacks do not clobber each other.

Parsing and safety: dns_resolve_* now take addr_cnt (clamped via esp_dns_clamp_addr_cnt()); the lwIP hook still passes MAX_ANSWERS (1) until lwIP forwards caller buffer size. Answer parsing can scan up to CONFIG_ESP_DNS_MAX_ANSWER_SCAN RRs while collecting IPs (helps CNAME chains); buffer size is CONFIG_ESP_DNS_BUFFER_SIZE. esp_dns_extract_ip_addresses_from_response is renamed to esp_dns_get_ips_from_response.

Examples, tests, CI: esp_dns_basic and a new esp_dns_concurrent stress app use net_connect instead of protocol_examples_common; cross-signed cert bundle verification and larger default getaddrinfo worker stack are documented and defaulted. CI builds both apps on updated IDF matrix versions.

^{Reviewed by Cursor Bugbot for commit 6b66578. Bugbot is set up for automated code reviews on this repo. Configure here.}

euripedesrocha

I did a partial review, but there are some discussions that might lead to structural changes.

euripedesrocha · 2026-06-01T13:46:23Z

-#define ESP_DNS_BUFFER_SIZE 512
+/** Maximum number of resource records to scan in a response */
+#ifndef ESP_DNS_MAX_ANSWER_SCAN
+#define ESP_DNS_MAX_ANSWER_SCAN 16


Should this be controlled by the user?

Added CONFIG_ESP_DNS_MAX_ANSWER_SCAN under Component config → ESP DNS (default 16).

euripedesrocha · 2026-06-01T13:46:39Z

-    err_t status;      /* Status of the answer */
-    ip_addr_t ip;      /* IP address from the answer */
-} dns_answer_storage_t;
+#define ESP_DNS_BUFFER_SIZE 512


Could be configurable as well.

Same pattern: CONFIG_ESP_DNS_BUFFER_SIZE in Kconfig (default 512 bytes).

euripedesrocha · 2026-06-01T13:49:27Z

- * @param response The DNS response to process.
- * @param ipaddr An array to store the extracted IP addresses.
+ * @param response The parsed DNS response
+ * @param ipaddr Array to store the copied IP addresses


Isn't better to have user to pass max number of entries?

dns_resolve_* now take addr_cnt; esp_dns_parse_response() and esp_dns_get_ips_from_response() take max_ips / max_entries, clamped via esp_dns_clamp_addr_cnt(). The lwIP hook still passes MAX_ANSWERS (1) until LWIP_HOOK_NETCONN_EXTERNAL_RESOLVE forwards the caller's buffer size.

euripedesrocha · 2026-06-01T13:51:13Z

    uint16_t id;                 /* Transaction ID */
    int num_answers;             /* Number of valid answers */
-    dns_answer_storage_t answers[MAX_ANSWERS];  /* Array of answers */
+    ip_addr_t answers[MAX_ANSWERS];  /* Array of IP addresses */


Is this a breaking change? I think we are under 1.0, but it is worth to guide users on this.

dns_response_t and the parse/extract helpers live in esp_dns_utils.h, which is component-private (PRIV_INCLUDE_DIRS). The public API is only include/esp_dns.h (init/cleanup + config). App code integrates via getaddrinfo() after esp_dns_init_*(); those signatures are unchanged. So this isn't a user-facing breaking change — no migration guide needed.

euripedesrocha · 2026-06-01T13:52:18Z

 - **Certificate Errors**:
  - Verify that the correct certificate is provided for secure protocols
  - For public DNS servers, use the certificate bundle approach
+  - If you see `No matching trusted root certificate found` when using the certificate bundle (e.g. with `dns.google` or Cloudflare), the server likely uses a cross-signed chain. You must enable `CONFIG_MBEDTLS_CERTIFICATE_BUNDLE_CROSS_SIGNED_VERIFY=y` in your `sdkconfig` (under Component config → mbedTLS → Certificate Bundle → Support cross-signed certificate verification).


How likely is this to happen? Should we have this set by default and let user to opt-out instead?

Cross-signed chains are common with public DoT/DoH when using the cert bundle. esp_dns can't default mbedTLS Kconfig from the component, but the examples and concurrent test ship with CONFIG_MBEDTLS_CERTIFICATE_BUNDLE_CROSS_SIGNED_VERIFY=y in sdkconfig.defaults. Updated the README to recommend the same for production projects and document how to opt out.

euripedesrocha · 2026-06-01T13:53:57Z


+/* ESP-IDF xTaskCreate() stack size is in bytes. DoT/DoH paths pull in mbedTLS inside
+ * getaddrinfo(); 4 KiB is too small and triggers a stack overflow on typical builds. */
+#define DNS_ADDRINFO_TASK_STACK 8192


Make it configurable?

Added CONFIG_EXAMPLE_DNS_ADDRINFO_TASK_STACK in the example's Kconfig.projbuild (default 8192 bytes). Tunable via menuconfig under the example options.

euripedesrocha · 2026-06-01T13:55:42Z


 * **Certificate Issues**:
  For DoT and DoH protocols, ensure that the certificates are valid for the DNS server you're using. The example includes Google DNS certificates, but these may need to be updated if they expire.
+  If you are using the Certificate Bundle and see a `No matching trusted root certificate found` error, it is likely due to cross-signed chains (e.g., from Google). Make sure that `CONFIG_MBEDTLS_CERTIFICATE_BUNDLE_CROSS_SIGNED_VERIFY=y` is enabled in your configuration. This can be enabled via `idf.py menuconfig` -> `Component config` -> `mbedTLS` -> `Certificate Bundle` -> `Support cross-signed certificate verification`.


Should it be set as default and let user opt out?

Enabled by default in this example's sdkconfig.defaults. Updated the troubleshooting text to say that instead of asking users to turn it on manually.

euripedesrocha · 2026-06-01T13:58:05Z

+| Supported Targets | ESP32 | ESP32-C2 | ESP32-C3 | ESP32-C5 | ESP32-C6 | ESP32-C61 | ESP32-H2 | ESP32-P4 | ESP32-S2 | ESP32-S3 |
+| ----------------- | ----- | -------- | -------- | -------- | -------- | --------- | -------- | -------- | -------- | -------- |
+
+This example stress-tests **`getaddrinfo()`** while the **esp_dns** component owns the lwIP **external resolve** hook. It is meant to surface concurrency bugs documented in [ISSUES_AND_PROGRESS.md](../../ISSUES_AND_PROGRESS.md):


Remove the reference of non-committed file.

Removed the ISSUES_AND_PROGRESS.md reference from the test README. That tracker is internal-only and is not part of the component.

euripedesrocha · 2026-06-01T13:58:28Z

@@ -0,0 +1,49 @@
+# esp_dns concurrent resolver test


If this is a test app, should be treated as one. Not on the examples.

Moved to tests/test_apps/esp_dns_concurrent and updated the README/CI accordingly.

euripedesrocha · 2026-06-01T13:59:29Z

+
+This example stress-tests **`getaddrinfo()`** while the **esp_dns** component owns the lwIP **external resolve** hook. It is meant to surface concurrency bugs documented in [ISSUES_AND_PROGRESS.md](../../ISSUES_AND_PROGRESS.md):
+
+- **Issue 1** — shared singleton resolver state (`response_buffer`, transaction id) under concurrent lookups.


Should we review the architecture and adopt a queue to the requests? This way we would fix the issu by removing the singleton.

Keeping the singleton for now. Per-query response_buffer on the stack (and DoH user_data) removes the shared mutable state that caused the races, without serializing lookups or adding a request queue. A full queue-based redesign would be a large API/architecture change, and involve a lot more analysis and testing. Will take it up for a future release.

Moved `response_buffer` out of the global handle context (`esp_dns_handle`) and into automatic/local storage within `dns_resolve_doh`, `dns_resolve_dot`, and `dns_resolve_tcp`. This prevents memory corruption and race conditions when multiple DNS queries are running concurrently. Also included `esp_dns_concurrent_test` example to validate concurrent lookup behavior.

Store ip_addr_t values directly in dns_response_t, rename esp_dns_extract_ip_addresses_from_response to esp_dns_get_ips_from_response, and scan up to CONFIG_ESP_DNS_MAX_ANSWER_SCAN RRs while collecting addresses. Add addr_cnt to dns_resolve_* and max_ips/max_entries to the parse/extract helpers. Add CONFIG_ESP_DNS_MAX_ANSWER_SCAN and CONFIG_ESP_DNS_BUFFER_SIZE.

Migrate esp_dns_basic to net_connect, make the getaddrinfo worker stack configurable, and enable cross-signed certificate verification in sdkconfig.defaults. Move the concurrent stress app to tests/test_apps/esp_dns_concurrent and add it to esp_dns CI builds.

abhik-roy85 self-assigned this May 4, 2026

abhik-roy85 added the dns label May 4, 2026

abhik-roy85 force-pushed the component/esp_dns_fix branch 2 times, most recently from 7a1adb3 to f2b75d9 Compare May 14, 2026 15:32

abhik-roy85 requested review from david-cermak and euripedesrocha May 14, 2026 15:44

abhik-roy85 force-pushed the component/esp_dns_fix branch from f2b75d9 to ec6a7b6 Compare May 15, 2026 10:48

euripedesrocha reviewed Jun 1, 2026

View reviewed changes

abhik-roy85 force-pushed the component/esp_dns_fix branch from ec6a7b6 to 135fa98 Compare June 11, 2026 12:31

abhik-roy85 force-pushed the component/esp_dns_fix branch from 135fa98 to a039ce1 Compare June 19, 2026 14:10

abhik-roy85 force-pushed the component/esp_dns_fix branch from a039ce1 to 3f7ca0f Compare July 7, 2026 14:09

abhik-roy85 added 3 commits July 8, 2026 21:17

abhik-roy85 force-pushed the component/esp_dns_fix branch from 3f7ca0f to 6b66578 Compare July 8, 2026 11:29


		This example stress-tests `getaddrinfo()` while the esp_dns component owns the lwIP external resolve hook. It is meant to surface concurrency bugs documented in [ISSUES_AND_PROGRESS.md](../../ISSUES_AND_PROGRESS.md):

		- Issue 1 — shared singleton resolver state (`response_buffer`, transaction id) under concurrent lookups.

Uh oh!

Conversation

abhik-roy85 commented May 4, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Component changes

Examples and tests

Related

Testing

Checklist

Uh oh!

euripedesrocha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

abhik-roy85 commented May 4, 2026 •

edited by cursor Bot

Loading